Skip to content

[Distillation] Fix eod masking + strategy refactoring#3478

Merged
copybara-service[bot] merged 1 commit intomainfrom
vladk/distill-eod-refactor
Mar 24, 2026
Merged

[Distillation] Fix eod masking + strategy refactoring#3478
copybara-service[bot] merged 1 commit intomainfrom
vladk/distill-eod-refactor

Conversation

@vlad-karp
Copy link
Copy Markdown
Collaborator

@vlad-karp vlad-karp commented Mar 20, 2026

Description

  1. The current code didn't correctly mask eod tokens when they are segment delimiters - sft mode was fine, but the pretraining incorrectly included that token into prediction.
    The last token in the sample still need to predict the eod token as expected, it is the oed input token which need to be excluded from the loss.

  2. The kl divergence averaging wasn't correct as well.

  3. Since this logic was in anonymous labels_fn function, it is hard to unit test it.
    Since we are not longer inheriting from tunix distillation strategy, we can add it directly to our custom strategy class.
    The base strategy class has been intrduced.

Tests

A new test test_strategy_ignores_segmentation_zero_tokens() has been added to tests/post_training/unit/train_distill_test.py

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code. For an optional AI review, add the gemini-review label.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed, including adding new documentation pages to the relevant Table of Contents (toctree directive) as explained in our documentation.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ners/post_train/distillation/distillation_utils.py 74.19% 7 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@gagika gagika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing it!

@vlad-karp vlad-karp force-pushed the vladk/distill-eod-refactor branch from ce96315 to 19d96eb Compare March 24, 2026 00:31
@vlad-karp vlad-karp force-pushed the vladk/distill-eod-refactor branch 3 times, most recently from ce5b87d to 03425b0 Compare March 24, 2026 18:53
@vlad-karp vlad-karp force-pushed the vladk/distill-eod-refactor branch from 03425b0 to ced6b20 Compare March 24, 2026 19:12
@copybara-service copybara-service Bot merged commit 9f6b09a into main Mar 24, 2026
31 checks passed
@copybara-service copybara-service Bot deleted the vladk/distill-eod-refactor branch March 24, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants